-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modules restructure #1859
Modules restructure #1859
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1859 +/- ##
==========================================
- Coverage 64.64% 64.49% -0.16%
==========================================
Files 34 34
Lines 4115 4137 +22
==========================================
+ Hits 2660 2668 +8
- Misses 1455 1469 +14
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge amount of work! 🙇🏻
Only made it a couple of files in but thought I'd submit now to get my questions in. Will try to read more but likely won't have time for a full review.
@@ -93,7 +93,8 @@ def init_mod_name(self, module): | |||
if self.repo_type == "modules": | |||
modules = self.get_modules_clone_modules() | |||
else: | |||
modules = self.modules_json.get_all_modules().get(self.modules_repo.fullname) | |||
modules = self.modules_json.get_all_modules().get(self.modules_repo.remote_url) | |||
modules = [module if dir == "nf-core" else f"{dir}/{module}" for dir, module in modules] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading quickly so can't see the full context, but why do we need special treatment for nf-core
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to get the module name suggestions, it will usually be nf-core, so we don't need to have nf-core/TOOL
, but in case you have modules from other repos I thought it would be nice to differentiate and have the other directory listed.
@@ -153,7 +156,7 @@ def get_local_yaml(self): | |||
|
|||
log.debug(f"Module '{self.module}' meta.yml not found locally") | |||
else: | |||
module_base_path = os.path.join(self.dir, "modules") | |||
module_base_path = os.path.join(self.dir, "modules", "nf-core") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, hardcoded nf-core
strings make me nervous 😅
checks are all green, that's good enough for me |
Apply the changes needed for the restructure of the nf-core/modules repository.
PR checklist
CHANGELOG.md
is updateddocs
is updated